-
Notifications
You must be signed in to change notification settings - Fork 773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ros2] gazebo_ros_force plugin #784
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and all tests pass, just some small comments
include_directories(include) | ||
include_directories( | ||
include | ||
${gazebo_dev_INCLUDE_DIRS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is needed? Thought include was added with ament_target_dependencies also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it fails to find the ign-math
includes for me
gazebo_plugins/CMakeLists.txt
Outdated
@@ -46,6 +65,11 @@ ament_package() | |||
install(DIRECTORY include/ | |||
DESTINATION include) | |||
|
|||
install(TARGETS gazebo_ros_force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just have one install target for all the plugins
install(TARGETS
gazebo_ros_force
gazebo_ros_template
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, done 0cc0e06
gazebo_plugins/CMakeLists.txt
Outdated
target_link_libraries(gazebo_ros_force | ||
${gazebo_ros_LIBRARIES} | ||
${geometry_msgs_LIBRARIES} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to make sure these lines and the include_directories lines are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work ok without the link libs now. I know I originally added them for a reason... Weird... The include still seems to be needed, but removed libs: 0cc0e06
Send an empty / zero message to stop applying a force. | ||
|
||
Example Usage: | ||
\verbatim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think \code{.xml}
shows up better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like it should, so changed 0cc0e06
impl_->wrench_msg_.force = | ||
gazebo_ros::Convert<geometry_msgs::msg::Vector3>(ignition::math::Vector3d::Zero); | ||
impl_->wrench_msg_.torque = | ||
gazebo_ros::Convert<geometry_msgs::msg::Vector3>(ignition::math::Vector3d::Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these will already be zeroed out by the default constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you're right, here's a handy reference. I'll update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Wait until box moves | ||
unsigned int sleep = 0; | ||
unsigned int max_sleep = 30; | ||
while (sleep < max_sleep && abs(box->WorldPose().Pos().X()) < tol) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there should be a larger tolerance for seeing that it moved. 10e-10 could just be the initial rounding error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, increased in 0cc0e06
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to merge after copyright dates adjusted
@@ -0,0 +1,83 @@ | |||
// Copyright 2012 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright year
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the lack of an end year, or to it being different from 2018? I think even after the refactoring, there's still enough left from the original plugin to keep the same initial year for each file.
Regarding the lack of a final year, take a look at this conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, thought end year was needed
@@ -0,0 +1,103 @@ | |||
// Copyright 2013 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Year
Example usage (for copy paste if anyone needs it in the future):
|
gazebo::ServerFixture
for testing